Migrate to pyproject.toml
#203
Conversation
Codecov Report
@@ Coverage Diff @@
## main #203 +/- ##
========================================
+ Coverage 70.5% 87.8% +17.2%
========================================
Files 16 9 -7
Lines 1880 1429 -451
Branches 288 0 -288
========================================
- Hits 1326 1254 -72
+ Misses 530 175 -355
+ Partials 24 0 -24
|
giddy/markov.py
Outdated
| @@ -300,7 +301,7 @@ def sojourn_time(self): | |||
| return self._st | |||
|
|
|||
|
|
|||
| class Spatial_Markov(object): | |||
| class Spatial_Markov: # noqa N801 – Class name should use CapWords convention | |||
There was a problem hiding this comment.
Leaving these comments in the code base does not seem to be the best idea. What is the best practice to "resolve" these comments? @jGaboardi
There was a problem hiding this comment.
This kind of comment (# noqa N801) is pretty standard practice for instances to ignore. I put the explanation in there for your information; it can be deleted.
Long term, the best solution is to update all class names to use CapWords-style (e.g. SpatialMarkov), but that would break a ton of stuff in the giddy API by doing that now.
Perhaps we can get @knaaptime's and @martinfleis's advice here.
There was a problem hiding this comment.
I'd ignore them in pyproject. Then you don't need these comments.
There was a problem hiding this comment.
This seems reasonable considering the circumstances. I'll update this PR @weikang9009.
There was a problem hiding this comment.
I've update the PR and move most of the # noqa ### to the pyproject.toml where they are connected with specific files. I do think it is important to keep the # noqa E501 (line too long) in place to cap lines at the desired length where possible. @weikang9009 but since you are the giddy BDFL if you don't think that's a good idea, I will move those, too.
There was a problem hiding this comment.
- There is, but seems to be convoluted and way too much effort, I'd vote for keeping the
# noqa E501here. - I explored this one and found that I would either be very difficult or not possible. I'd also vote for keeping
noqa UP031in here, too. Or I can addUP031to topyproject.tomlto ignore specifically inmarkov.py. Would that be a good compromise?
There was a problem hiding this comment.
Can we move both E501 and UP031 to pyproject.toml since there are no easy ways to address them at the moment?
There was a problem hiding this comment.
Moving UP031 is reasonable since it happens only in 1 file, we know where it happens, and it is not likely to happen anywhere else (creating strings of .tex files). However, long lines is something much more common and very much affects readability. Generally, this is a trivial thing to fix in code, comments, and docstrings. But the offending lines here are actual print outs within doctests (in ergodic.py, markov.py, and sequence.py). The means the actual print commands need to change for a "proper fix".
In terms of what I would recommend in order of (what I see as) best practice:
- Keep the
# noqa E501at the end of offending docstrings. It's whatruffrecommends. - Change the
printstatements in the code base so that no line is longer than our enforced length. This "fixes" the line length issue, but then were are changing actual code to fix something in a doctest print out. It doesn't feel right to me. - Add a global
# noqa E501inpyproject.tomlonly forergodic.py,markov.py, andsequence.py. Sinceblackalso handles line length, this takes care of many offenders but not all (blackhas a tough time with long strings).
So, I've said my piece 😆 and I will defer to your judgement @weikang9009. Will it be 1, 2, or 3? Whichever you choose, I will implement here.
There was a problem hiding this comment.
the current cases of
# noqa E501are long lines that are error messages in the docstrings. I am unsure whether there is a way to make them conform to the 79-character rule. Any thoughts?
See https://github.com/geopandas/geopandas/blob/e5bcf27c157b483ead2a7e63a280bf90489762c4/geopandas/geodataframe.py#L538-L549 on how it can be done with a little effort. Alternatively, we could keep the local # noqa E501 there. We can also just ignore E501 and add it to pyproject but the fix seems easy enough.
there are a bunch of noqa UP031 in markov.py that are used for outputting a latex table. I have explored a bit, and I am not sure how to make them conform to the printf-string-formatting rule.
I'd just keep the # noqa there .
Change the print statements in the code base so that no line is longer than our enforced length. This "fixes" the line length issue, but then were are changing actual code to fix something in a doctest print out. It doesn't feel right to me.
No, we should not enforce line breaks in prints/warnings to conform to a rule we impose on our code. It can look weird in notebooks etc.
My proposal is to fix the line lengths with a line break as shown in the geopandas example and keep # noqa UP031 in the code.
There was a problem hiding this comment.
Thank you for the geopandas example @martinfleis! I was able to fix those long lines (just pushed the commit that fixes them to this PR).
@jGaboardi Let's go with your option 1. Thank you so much!
This PR resolves #200 and performs more rigorous linting, clean up, etc. Also, with added doctesting coverage goes up to 88%.